Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: improve worker_threads documentation #26110

Closed
wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

This adds a few examples and clarifications.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

This adds a few examples and clarifications.
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Feb 14, 2019
@addaleax addaleax added the worker Issues and PRs related to Worker support. label Feb 14, 2019
doc/api/worker_threads.md Outdated Show resolved Hide resolved
@@ -156,6 +209,9 @@ Disables further sending of messages on either side of the connection.
This method can be called when no further communication will happen over this
`MessagePort`.

The [`'close'` event][] will be emitted on both `MessagePort` instances that
Copy link
Member

@Trott Trott Feb 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be I'm over-reacting to passive voice, but perhaps this?:

Both `MessagePort` instances that are part of the channel will emit a [`'close'` event][].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit that active voice sounds a bit weird to me when referring to events :)

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left two completely-optional totally-ignorable comment/suggestions, but LGTM as-is.

@Trott
Copy link
Member

Trott commented Feb 15, 2019

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 15, 2019
@@ -91,6 +120,16 @@ added: v10.5.0
An arbitrary JavaScript value that contains a clone of the data passed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to include more of a description of what "clone" here means in general. I know there are a few bits and pieces of an explanation through the doc, but coalescing those into a single section with some information on what types of values cannot be cloned, would be good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell How does 919fd07 sound to you?

The `worker_threads` module enables the use of threads with message channels
between them. To access it:
The `worker_threads` module enables the use of threads that execute JS code
in parallel. To access it:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something more like "that allow parallel execution contexts for JS code"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fishrock123 I’m not sure … “execution context” sounds like a very generic thing?

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With nits)

doc/api/worker_threads.md Outdated Show resolved Hide resolved
doc/api/worker_threads.md Outdated Show resolved Hide resolved
doc/api/worker_threads.md Outdated Show resolved Hide resolved
doc/api/worker_threads.md Outdated Show resolved Hide resolved
doc/api/worker_threads.md Outdated Show resolved Hide resolved
@addaleax
Copy link
Member Author

@vsemozhetbyt Done!

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2642/

@addaleax
Copy link
Member Author

Landed in 70a500f

@addaleax addaleax closed this Feb 17, 2019
@addaleax addaleax deleted the worker-doc branch February 17, 2019 18:00
addaleax added a commit that referenced this pull request Feb 17, 2019
This adds a few examples and clarifications.

PR-URL: #26110
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
addaleax added a commit that referenced this pull request Feb 17, 2019
This adds a few examples and clarifications.

PR-URL: #26110
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
rvagg pushed a commit that referenced this pull request Feb 28, 2019
This adds a few examples and clarifications.

PR-URL: #26110
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants